One partition's failure no longer ends the maintenance walk (#737) - #747

neutral review — round 1

Head reviewed: 84fca7f9af6a934df43211ba88a4044a6b3e8095. All 9 checks green at
that SHA. Reviewed in a detached worktree; nothing was written to the branch.

VERDICT: findings — 1 major, 3 minor.

The core change holds up. The catch is genuinely outside withSpan, so the
failing partition's span still records ERROR. LLP 0217's stamp cannot be
pre-empted: stampWriterGeneration fires from compactGeneration's own catch
before the rethrow, verified for all three orderings (throw after the stamp
write, the stamp write itself throwing, throw before compaction is attempted).
failed and compactionAttemptFailed do not co-occur in any ordering the
reviewer could build, including a partition carrying a stamp from a previous
tick that grows and throws again.

1. major — src/core/cache/maintenance.js:153: a failed partition satisfies the "always work one partition" budget guarantee, so the walk can still be starved by the first partition

The guard is unchanged from before this PR:

if (reports.length > 0 && Date.now() - startMs > budgetMs) break

Before this PR a throwing partition never reached reports.push, so
reports.length > 0 could only mean "one partition was actually maintained".
Now a failed partition is pushed like any other, so it satisfies the
precondition having done no work. If elapsed time has passed budgetMs by the
time partition 0's failure is recorded, the loop breaks and every partition
behind it gets neither compaction nor snapshot expiry — the exact outcome #737
was filed about, reached by a different route.

Reproduced with this PR's own fixture:

maintainCache({ compactOnly: true, budgetMs: 0 })
partitions: [{"d":"ai_gateway_messages","failed":true,"compacted":false}]
totalFailed: 1  totalCompacted: 0

The healthy logs partition behind it is never visited.

Not hypothetical: the daemon passes budgetMs: mCfg.max_tick_ms (30s default),
and the code's own comment at :150-152 asserts the ranking pass can already
exceed the budget on a large cache. It also recurs every tick on the most
common path — a partition attempted via the growth gate rather than the
stale-verdict gate writes no LLP 0217 stamp on failure (compactGeneration's
catch is guarded by if (verdictStale)), so it is re-attempted and fails tick
after tick:

PROBE growth t1: failed=true ; cursor.compaction after t1: null
PROBE growth t2: failed=true
PROBE growth t3: totalFailed = 1

(The missing stamp on the growth path is pre-existing LLP 0217/0218 territory,
not this PR's to fix — but it is what turns this from a one-tick blip into
permanent starvation.)

This directly falsifies LLP 0220's unconditional consequence:

A cache holding one permanently broken partition still gets full maintenance
everywhere else, on the tick the breakage happens and on every tick after it.

Fix — make the guarantee about work done, not about reports existing:

  let totalFailed = 0
+ let maintained = 0

  for (const part of partitions) {
-   if (reports.length > 0 && Date.now() - startMs > budgetMs) break
+   // A partition that threw did no work, so it must not be the one that
+   // satisfies this guarantee: a first partition that fails slowly would
+   // otherwise starve the walk behind it on every tick, which is #737.
+   if (maintained > 0 && Date.now() - startMs > budgetMs) break
    ...
    reports.push(report)
+   if (!report.failed) maintained++

Note the tradeoff and settle it deliberately in LLP 0220: with that change, a
cache where every partition fails walks past the budget. If unbounded overrun
matters, add a second disjunct. Either way LLP 0220 should state which
guarantee actually holds, and a test should pin it.

2. minor — src/core/cache/maintenance.js:432: rebaselined is set before the cursor write that persists it

r.rebaselined = true                                   // :432
getActiveSpan()?.setAttribute('rebaselined', true)
if (!opts.dryRun) {
  await writeCursor(r.path, rebaselineCursor(cursor, dataFilesBefore))   // :439 — can throw
  rebaselinesCounter.add(1, ...)
}

Pre-PR a writeCursor throw here rejected the tick and the report was
discarded, so nobody read the flag. Post-PR the report survives: the partition
comes back rebaselined: true and failed: true, the CLI prints
rebaselined to N files (foreign sorted replace), FAILED: …, and :221
increments totalRebaselined — the summary claims a rebaseline that is not on
disk, and the partition is re-rebaselined next tick, so the count is also
double-reported.

This is the one place where the new "report mutated in place, catch
mid-mutation" pattern yields a field asserting persistence it does not have.
compacted, snapshotsExpired and compactionIneffective are all set after
their write or read no state.

Fix: move r.rebaselined = true to after the writeCursor succeeds. Keep the
span attribute where it is if the intent is worth recording on a span that will
carry ERROR anyway; the report field is the one that must not lie.

3. minor — src/core/commands/query.js:351: on a degraded tick the failure text now goes only to stdout

Previously the exception propagated to bin/hypaware.js:70, which wrote
hyp: <message> to stderr and exited 1. The exit code is preserved
(correctly 0 on a clean tick, and --dry-run over a torn partition yields
totalFailed: 0 so it still exits 0), but the diagnosis is written with
ctx.stdout.write only. hyp query maintain >/dev/null, or any cron/systemd
wrapper that mails or logs stderr, now gets an empty stderr on a run that lost
a partition — the "goes somewhere quieter" risk, on the one channel the PR did
not audit.

Fix, after the summary line and before the return:

if (report.totalFailed > 0) {
  ctx.stderr.write(`hyp query maintain: ${report.totalFailed} partition(s) failed; the walk continued\n`)
}
return report.totalFailed > 0 ? 1 : 0

4. minor — src/core/daemon/runtime.js:699: setAttribute('status', 'degraded') does not change the tick span's status code

withSpan (src/core/observability/span_helpers.js:29-37) derives span status
from sanitized, the attribute snapshot taken at span creation, and calls
setStatus({ code: OK }) after the callback returns. The tick opens with
status: 'ok', so setting the attribute inside the callback is honoured as an
attribute but the exported span status is OK (an explicit in-callback
setStatus(ERROR) would be clobbered too). Pre-PR a partition failure made
maintenance.tick an ERROR span; now it is an OK span with status="degraded".

The signal is not lost — the maintenance.partition span still carries
recordException + ERROR, and the per-partition daemon.maintenance_failed
lines are new and good — but the repo's convention is that a non-ok status
attribute is the span status, and this is the one place it silently isn't.
LLP 0220:81 ("marks the maintenance.tick span status: degraded") reads as
if the status code moves.

Fix: either teach withSpan to re-read the live status attribute off the span
before setting status (kernel change, benefits every caller with a post-hoc
verdict), or drop the status: 'ok' seed for this span and state in LLP 0220
that only the attribute carries the degradation, so no reader expects an ERROR
span.

Where a failure could still go quiet

Every path traced from "a partition's work throws" to "someone can see it":

• Per-partition compaction throw → report. Covered.
  errorKind: "maintenance_partition_failed", errorMessage carries the real
  text. Note error_kind is a constant for every real error here — no thrown
  error in this path carries .errorKind — so the useful content is message.
• Daemon reads the report. Covered, runtime.js:688-702, one
  daemon.maintenance_failed per failed partition with dataset/partition/kind/
  message. Attr.ERROR_KIND is imported and defined.
• Daemon outer .catch still covers what is genuinely outside. Verified:
  discoverCachePartitions, the unbudgeted ranking pass, cleanRetiredEpochs,
  and the final report construction are all outside the try and still reject.
  The comment at :706-707 is honest.
• Anything else reading totalFailed. maintainCache has exactly two
  production callers (query.js:301, runtime.js:672); both read it. Nothing
  in daemon/status.js, commands/status.js or query/overview.js surfaces
  maintenance, so no third consumer is left stale. totalFailed is required on
  MaintenanceReport and typecheck is clean, so no construction site was missed.
• Expire-phase failure rather than compaction. expireSnapshots
  (:559-563, :587-590) swallows its own errors and returns 0, so an expiry
  failure is invisible today. Pre-existing and untouched; if it did throw it
  would land in the new catch correctly. Worth its own issue, out of scope here.
• The budget break. Partitions the break skips are simply absent from the
  report, with no field saying the walk was cut short. Pre-existing for the
  clean case (LLP 0199), newly reachable via a failure — finding 1.
• Loop state mid-mutation. compacted: true + failed: true is
  unreachable. snapshotsExpired is correctly kept on a partition that later
  throws (test 3 pins it). rebaselined is the exception — finding 2.

Also checked, clean

• LLP 0220. Claims match the code (with the wording caveats in findings 1
  and 4). Both code-referenced anchors resolve; Extended-by forward-refs on
  0199 and 0218 are purely additive; nothing Accepted is rewritten. 0219 is free
  in this tree, as claimed.
• Conventions. Zero U+2014 in all 11 changed files. No stray semicolons. No
  @typedef added (the one at :261 is pre-existing). No inline import('...')
  types. Type-import specifiers in the new test are root-anchored .js.
• Tests. cache-maintenance-walk-resilience.test.js discriminates — the
  fixture puts a genuinely healthy partition behind the failing one in
  neediest-first order. The four assert.rejects → partitions[0].failed
  conversions preserve what they pinned. Gaps below the finding bar: nothing
  covers the daemon's per-partition logging, the CLI's exit-1 path, or the
  budget interaction (finding 1).
• Ran (fresh npm install): npm run typecheck clean; npm test 4006
  tests, 4005 pass, 0 fail, 1 pre-existing skip; targeted node --test over six
  cache suites + llp-ref-hygiene, 73/73; smokes cache_lifecycle_maintenance
  and incremental_sink_compaction both ok.
• No other consumer of the old exit contract. hyp query maintain is not
  invoked from the wizard, walkthrough, sync, or any smoke other than
  cache_lifecycle_maintenance's dry-run-exits-0 check.

Round 1 of 2. A fix round follows; round 2 reviews the result.

VERDICT: findings

neutral review - round 2 (final)

Head reviewed: fb63863d7141fb8044f40eba2dce1e77a42e780b, parent 84fca7f. Reviewed in a detached worktree; nothing written to the branch, git status clean at exit.

VERDICT: findings - 0 blockers, 0 major, 1 minor, 4 nits. Nothing here is ship-blocking. All four round-1 findings are genuinely fixed, and I could not reproduce any of them at this head. The one minor and the nits are precision/robustness items that a follow-up can absorb.

Priority 1 was the withSpan bypass, and it is the round's best outcome: I enumerated span_helpers.js line by line against the rewrite and the bypass drops nothing. Verified empirically as well as by reading (probe below).

1. minor (not blocking) - llp/0220-maintenance-walk-survives-a-partition.decision.md:115-119: the consequence understates when MAX_FAILURES_BEFORE_BUDGET_BREAK fires, and leaves the residual starvation window undocumented

The doc says:

The one case that guard alone cannot bound - every partition in the cache failing - is capped separately: the walk still breaks once MAX_FAILURES_BEFORE_BUDGET_BREAK partitions have failed past the budget, so an all-failing cache cannot walk unbounded either.

The cap is not scoped to an all-failing cache. maintenance.js:180-183 breaks as soon as totalFailed >= 4 and the budget is past, regardless of how many healthy partitions are queued behind. So in a 400-partition cache where four partitions fail, the other 396 are cut off exactly as they were before this PR.

That is not hypothetical for the shape #737 describes. Round 1 established that a partition attempted via the growth gate writes no LLP 0217 stamp on failure (compactGeneration's catch is guarded by if (verdictStale), maintenance.js:504), so growth-path failures recur every tick rather than converting into a cheap compactionAttemptFailed skip. Four such partitions, each failing slowly (a 318MB rewrite that throws late is the #723 profile), plus the unbudgeted ranking pass, is enough to blow max_tick_ms and re-establish permanent starvation of everything behind them.

This is a documented, deliberate, tested tradeoff - round 1 asked for exactly that, and the code half is right. What is wrong is only that the doc reads as if the cap is a corner-case guard for an all-failing cache, when it is in fact the boundary of the guarantee this LLP makes.

Fix, replacing the sentence at :115-119:

  The one case the "maintained > 0" gate alone cannot bound - a walk that
  keeps finding failures and never reaches a partition it can maintain - is
  capped separately: the walk breaks once `MAX_FAILURES_BEFORE_BUDGET_BREAK`
  (4) partitions have failed past the budget. That cap is what the guarantee
  stops at: a cache with four or more partitions failing per tick, on a tick
  that has already spent `max_tick_ms`, still leaves the partitions behind
  them unvisited. Bounding the tick and guaranteeing every partition a turn
  are not simultaneously satisfiable here, and this document chooses the
  bound.

No code change needed.

2. nit - src/core/daemon/runtime.js:677-684: the span prologue is now outside the .catch that used to cover it

getTracer('daemon'), buildAttrs({...}) and tracer.startActiveSpan's synchronous prologue (normalizeAttributes, new Span(...), randomHex) run before the try and outside the .catch(...) at :734, which is attached only to startActiveSpan's return value.

Under withSpan this was total coverage for free: withSpan is an async function, so calling it can never throw synchronously, and every error inside it - including its own prologue - surfaced as a rejection the .catch handled. Now a synchronous throw in the prologue rejects runMaintenance(), and the caller at :744 is maintenanceInFlight = runMaintenance().finally(...) with no catch anywhere, so it becomes an unhandled rejection in a long-lived daemon.

Practically unreachable (none of those calls has a realistic throw path), which is why this is a nit and not a finding of substance. But it is the one thing the bypass gave up relative to the helper, and it costs two lines:

     async function runMaintenance() {
+      try {
         const tracer = getTracer('daemon')
         ...
-      }).catch((err) => {
+        })
+      } catch (err) {
         const message = err instanceof Error ? err.message : String(err)
         fileLog.error('daemon.maintenance_failed', { message })
-      })
+      }
     }

3. nit - test/core/daemon-maintenance-tick-status.test.js:83: a partially flushed JSONL line makes the poll throw instead of retrying

pollJsonlFor catches only ENOENT and rethrows everything else (:86-88), but JSON.parse(line) at :83 is inside that same try. The exporter writes to an fs.WriteStream that this test never flushes (the test's own comment at :66-69 says so), so a read landing mid-write yields a truncated trailing record and a SyntaxError that escapes the poll loop as a hard test failure rather than another 25ms retry. Low probability at these record sizes, but it is the only unbounded-failure mode left in an otherwise well-behaved test.

Fix:

       for (const line of raw.split('\n')) {
         if (!line) continue
-        const record = JSON.parse(line)
-        if (predicate(record)) return record
+        // A line still being flushed reads as truncated JSON; retry rather
+        // than fail the poll on it.
+        let record
+        try { record = JSON.parse(line) } catch { continue }
+        if (predicate(record)) return record
       }

4. nit - src/core/daemon/runtime.js:729: attrs.error_kind ?? 'unhandled_exception' is dead

attrs is buildAttrs over a four-key literal that never contains error_kind, so the left operand is always undefined. It is a faithful copy of span_helpers.js:44, where the same expression is at least reachable through caller-supplied attributes. Harmless; either drop to the literal 'unhandled_exception' or leave it as deliberate parity with the helper. Not worth a round.

5. nit - src/core/cache/maintenance.js:249 vs llp/0220:107-108: maintained counts "did not throw", not "did work"

if (!report.failed) maintained++ increments for a partition that was skipped (LLP 0218 compactionAttemptFailed), a no-op, or a dry-run preview. The doc says the guard is "keyed off partitions actually maintained, not partitions merely visited", which reads stronger than the code does.

Behaviourally this is right and I would not change it: a skip or a no-op is instant, so it cannot consume the budget the way a slow failure can, and counting them matches the pre-PR reports.length > 0 semantics that LLP 0199 settled. Only the doc sentence overclaims. Suggested wording: "keyed off partitions the walk got through, not partitions that merely produced a report - a partition that threw did no work, so it cannot itself satisfy the guard's guarantee".

6. nit - test/core/query-maintain-cli.test.js:117: the stderr assertion is a full-line exact match

/^hyp query maintain: 1 partition\(s\) failed; the walk continued$/m pins prefix, count, phrasing and trailing clause. A pure reword of an operator-facing string breaks the test for nothing. Pinning the count is worth keeping; the rest is not. /^hyp query maintain: 1 partition\(s\) failed\b/m keeps everything the test is actually for. Judgement call, listed for completeness.

Round-1 findings, re-derived

1. major, failed partition satisfying the budget guarantee - FIXED. maintenance.js:180-183 now gates the break on maintained > 0 || totalFailed >= MAX_FAILURES_BEFORE_BUDGET_BREAK, and maintained (:249) only counts non-failed partitions. Re-ran round 1's own reproduction: maintainCache({ compactOnly: true, budgetMs: 0 }) over the torn-plus-healthy fixture now visits both, and logs comes back compacted: true. totalFailed is read at the top of iteration N and counts iterations 0..N-1 only, which is the right quantity given the report is pushed after the catch. The budgetMs ?? Infinity default means neither disjunct ever fires from the CLI. Caches of 1/2/3 partitions never reach the cap and simply walk to the end, which is correct. Both new tests discriminate against the round-1 head (the starvation test would see compacted unset; the cap test would see totalFailed: 1 instead of 4), and neither depends on timing: the zero-budget guard is satisfied by the first Date.now() delta after discovery in every ordering. Residual: finding 1 above, doc-only.
2. minor, rebaselined set before the write that persists it - FIXED. maintenance.js:471-482: r.rebaselined = true now sits after await writeCursor(...) in the non-dry-run branch, before rebaselinesCounter.add. The dry-run else branch setting it immediately is correct - nothing is written, so nothing can fail, and it matches r.compacted = true at :484 for the ordinary rewrite path; totalRebaselined on a dry run keeps meaning "would rebaseline", the same reading totalCompacted already had, and the existing preview test (cache-retention-maintenance.test.js:889-892) still pins it. The span attribute is deliberately left before the write with a comment explaining why, which is what round 1 allowed. The new test's chmod 0o555 restore is in an inner finally, so permissions are returned even when the assertions fail, and the outer finally still rm -rfs. As root, chmod 0o555 would not block the write and the test would fail loudly on its own 'fixture invariant' assertion rather than pass vacuously; the repo already relies on this pattern twice in cache-compaction-effectiveness.test.js:415,468 (pre-existing, confirmed via git diff), so it is not a new environmental assumption.
3. minor, degraded tick silent on stderr - FIXED. query.js:360-363 writes the line after the summary and before the return. Exit codes unchanged and pinned: 1 degraded, 0 clean (query-maintain-cli.test.js:114,141). --dry-run is unaffected - the dry-run paths never throw, so totalFailed stays 0 and the exit stays 0; the cache_lifecycle_maintenance smoke's dry-run-exits-0 check still passes. The text names the command, the count, and that the walk continued, which is what a cron/systemd wrapper needs. Wording brittleness is nit 6.
4. minor, setAttribute('status','degraded') not moving the status code - FIXED, and the bypass is faithful. I enumerated span_helpers.js:27-50 against the rewrite: tracer selection with the same component ('daemon', matching the old { component: 'daemon' }), buildAttrs sanitization, startActiveSpan(name, {attributes}, fn) so the active-context parent is established, recordException on throw, setStatus(ERROR, err.message) on throw, the error_kind attribute on throw, throw err to preserve propagation to the outer .catch, span.end() in a finally so it runs on the success path, the throw path, and the report-handling path alike, and an explicit status on success. Nothing in the helper is unaccounted for except the prologue coverage in nit 2. Verified empirically with a probe daemon over 24 real ticks: the tick span exports status: "failed", statusMessage: "1 partition(s) failed", attributes.status: "degraded", partitions_failed: 1, partitions_maintained: 0; every single maintenance.partition span's parentSpanId resolves to a maintenance.tick spanId (so the bypass did not detach the children), and every tick span has an endTimestamp with durationMs >= 0 (so no span is leaked unended, on either the clean or the degraded path). The exporter maps SpanStatusCode.ERROR to the string "failed" (jsonl_exporters.js:113-119), which is what the new test asserts, and it is "ok" under the round-1 head, so the test discriminates.

Also checked, clean

• The new daemon test as a CI citizen. Bounded 5s poll with a useful failure message ('the maintenance.tick span must be exported within the poll window'); pollJsonlFor returning undefined fails on assert.ok, so it cannot pass vacuously on a missing or empty file. It cannot match the wrong tick: spans are appended in end order, the torn partition is present from t=0, and the first maintenance.tick record in the file is therefore always the degraded one (later ticks go clean once LLP 0217's stamp converts the partition into a skip - confirmed in the probe: ticks 2..24 export status: "ok"). Cleanup restores both env vars and rm -rfs HYP_HOME in a finally; the daemon is stopped via handle.stop() + await handle.done; no child process is spawned (runDaemon runs in-process) and no port is bound. Repeat runs: 6 sequential runs, all pass, 232-256ms each - roughly 20x headroom inside the 5s poll budget - plus 4 concurrent runs, all pass, so it survives node --test's per-file process parallelism. No temp directories left behind after 10 runs (hypaware-daemon-maint-status-*, hyp-maintain-cli-*, hyp-maintain-all-failing-* all zero). interval_minutes: 0.001 passes the v2 validator (schema.js:634-642 accepts any finite non-negative number), so the 60ms interval is real and not silently defaulted to 60 minutes. One residual: installObservability is a module-level singleton that runDaemon's shutdown never tears down, so the JSONL WriteStream stays open past the test - harmless here (one test per file, per-process isolation) and pre-existing.
• Round-1-cleared items, re-checked after the runMaintenance rewrite. The LLP 0217 stamp is untouched (maintenance.js:504-515, still inside compactGeneration's catch, before the rethrow). failed and compactionAttemptFailed still cannot co-occur: compactionAttemptFailed is only set in the !shouldCompact branch (:437-451) and every throw path is inside if (shouldCompact) (:452+). The daemon's outer .catch still covers exactly what it claims - the inner catch rethrows, so discoverCachePartitions, the unbudgeted ranking pass, cleanRetiredEpochs and report construction all still reach daemon.maintenance_failed with the message. The tick span on that path carries status: "ok" as an attribute alongside an ERROR code, which is precisely what withSpan produced before, so nothing regressed there either.
• LLP 0220. Both code-referenced anchors (#walk-survives-a-partition, #tick-reports-degraded) resolve; the #tick-reports-degraded section now carries the withSpan-bypass rationale that runtime.js:665-675's [constrained-by] ref points at, and that ref is correctly attached (no blank line before async function runMaintenance). The falsified consequence bullet is rewritten rather than deleted. Extended-by forward-refs on 0199 and 0218 are purely additive one-liners; nothing Accepted is rewritten. 0220 is still the only claim on that number in this tree (0219 remains unused - a numbering gap, not a collision). test/core/llp-ref-hygiene.test.js 11/11.
• Conventions. Zero U+2014 across all 14 changed files. No stray semicolons (the seven ;$ hits in changed files are all mid-comment prose). No @typedef added - the one at maintenance.js:292 is pre-existing. No inline import('...') types; the four await import(...) hits are dynamic module loads. Type-import specifiers in both new test files are root-anchored .js (../../src/core/cache/types.js, ../../hypaware-plugin-kernel-types.js).
• Ran (after a fresh npm install): npm run typecheck clean. npm test 4012 tests, 4011 pass, 0 fail, 1 pre-existing skip. Targeted node --test over cache-maintenance-walk-resilience, query-maintain-cli, cache-retention-maintenance, cache-compaction-effectiveness: 56/56. llp-ref-hygiene: 11/11. daemon-maintenance-tick-status: 10/10 across sequential and parallel runs. Smokes cache_lifecycle_maintenance and incremental_sink_compaction both ok (both gained a totalFailed === 0 assertion, which is the right shape now that a failure no longer rejects).

Round 2 of 2. Triage decides ship-or-block from here; my read is that nothing above justifies blocking, and findings 1 through 6 are follow-up material.

neutral review record - round 1 (marker correction)

This comment exists to carry the round marker in the form the reconciler counts.
The round itself already happened and its full record is
here:
I signed that comment <!-- neutral-review round=1 pr=747 head=... -->, which is
not the contract (<!-- neutral-review: <sha> <clean|findings> -->), so the CLI
counted zero rounds and would have re-reviewed the same head indefinitely. My
error, corrected here rather than by editing the original.

Head reviewed: 84fca7f9af6a934df43211ba88a4044a6b3e8095. Verdict: findings
(1 major, 3 minor), all four fixed in fb63863:

1. major - src/core/cache/maintenance.js:153: the budget guard let a failed
   partition satisfy the "always work one partition" guarantee, so a slow-failing
   first partition still starved the walk behind it. maintainCache aborts the whole walk when one partition's compaction throws #737 by another route,
   reproduced with the PR's own fixture.
2. minor - maintenance.js:432: rebaselined set before the writeCursor
   that persists it, so a failed tick reported and counted a rebaseline that never
   landed on disk.
3. minor - src/core/commands/query.js:351: the degraded-tick message went
   only to stdout; stderr was silent where it used to carry the failure.
4. minor - src/core/daemon/runtime.js:699: setAttribute('status', 'degraded') never moved the span's status code, so a degraded tick exported an
   OK span.

Full evidence, reproductions and suggested diffs are in the linked comment.

neutral review record - round 2, final (marker correction)

Same correction as the round-1 record above: the full round-2 review is
here and
was signed with a marker the CLI does not parse. This comment carries the
contract-form marker so the round counts.

Head reviewed: fb63863d7141fb8044f40eba2dce1e77a42e780b. Verdict: findings

• 0 blockers, 0 major, 1 minor, 4 nits. Nothing ship-blocking.

All four round-1 findings were re-derived independently and are genuinely fixed;
none reproduced at this head. The priority item was the fixer's answer to round-1
finding 4, which bypassed withSpan and hand-rolled startActiveSpan in
runMaintenance - a larger structural move than the finding asked for. Enumerated
against span_helpers.js line by line, the bypass drops nothing: tracer selection,
buildAttrs sanitization, active-context parenting, recordException,
setStatus(ERROR), the error_kind attribute, rethrow, and span.end() in a
finally are all reproduced. Confirmed empirically over 24 real ticks: every
maintenance.partition span's parentSpanId resolves to a maintenance.tick
spanId (children did not detach), and every tick span carries an endTimestamp
(no leaked spans on either path).

Residual, all follow-up material:

1. minor - llp/0220:115-119: the consequence reads as if
   MAX_FAILURES_BEFORE_BUDGET_BREAK is a corner-case guard for an all-failing
   cache. It is not scoped that way: four failures past the budget cut the walk off
   regardless of how many healthy partitions are queued behind. That is the actual
   boundary of the guarantee this LLP makes, and the doc should say so. Code is
   correct; wording only.
2. nit - runtime.js:677-684: the span prologue now sits outside the .catch
   that used to cover it, so a synchronous throw there would become an unhandled
   rejection in a long-lived daemon. Practically unreachable; two lines to close.
3. nit - test/core/daemon-maintenance-tick-status.test.js:83: a partially
   flushed JSONL line makes the poll throw instead of retrying.
4. nit - runtime.js:729: attrs.error_kind ?? 'unhandled_exception' is dead.
5. nit - maintenance.js:249 vs llp/0220:107-108: maintained counts "did
   not throw", not "did work". Behaviour is right; the doc sentence overclaims.
6. nit - test/core/query-maintain-cli.test.js:117: the stderr assertion is a
   full-line exact match, so a pure reword breaks it for nothing.

Review budget is now exhausted at 2 rounds. Triage decides ship-or-block next.

neutral triage - ship

Review budget exhausted at 2 rounds with six residual findings open, so triage
judged each as a true blocker (a production defect: wrong behaviour, data loss, a
security hole, a crash, a perf regression past budget) or a preference.
All six are preferences. This PR can merge.

Deferred to #753, which also picks up a seventh, pre-existing item.

Two of the six were checked rather than inherited, because a mis-labelled nit is
exactly what this rung exists to catch:

• The "unhandled rejection in a long-lived daemon" nit was the one that could
  have been a blocker. The gap is real: runMaintenance is async, the .catch
  is attached only to startActiveSpan's promise, the caller is .finally(...)
  with no catch, and there is no unhandledRejection handler anywhere in
  src/ or bin/, so a reachable throw would take the daemon down. Triage then
  enumerated every operation in the uncovered region (getTracer's two field
  assignments, buildAttrs over a fixed four-key literal with normalizeValue's
  only throwing branch already caught, startActiveSpan's prologue, new Span
  whose nowHrTime is finite by spec, and randomHex calling
  crypto.randomBytes). The only theoretical throw is an entropy failure, which
  would already be breaking every span in the process. Unreachable, so:
  preference, and cheap hardening in the follow-up.
• maintained counting "did not throw" rather than "did work" could have
  weakened the starvation guarantee if a skip could be slow. It cannot: in the
  daemon path every non-throwing partition runs expireSnapshots, which is real
  work; the cursor-based skips read only the cursor; the one zero-work path is the
  !tableExists early return, a single stat. A non-throwing partition either did
  maintenance or finished in microseconds, so it cannot consume the budget the way
  a slow failure can. The behaviour is right and only the LLP sentence overclaims.

On the cap finding, the important judgement is that it is the boundary of a
strict improvement, not a regression. Before this PR, one slow-failing
growth-path partition starved everything behind it, every tick, forever. After it,
that takes four concurrent failures on a tick that has already blown
max_tick_ms. Bounding the tick and guaranteeing every partition a turn are not
simultaneously satisfiable; round 1 asked for the bound explicitly and it is
tested. The real mitigation target is the pre-existing growth-path stamp gap
(maintenance.js:504), which is item 6 on #753.

Verified at head fb63863 before deciding: npm run typecheck clean, npm test
4011 pass / 0 fail / 1 pre-existing skip, smokes cache_lifecycle_maintenance and
incremental_sink_compaction both ok.